C++: Split new range analysis into constant and relative stages#11928
C++: Split new range analysis into constant and relative stages#11928MathiasVP merged 10 commits intogithub:mainfrom
Conversation
|
I see there are some result changes caused by this PR. That's not expected, is it? |
7e363fa to
d4e3f7f
Compare
MathiasVP
left a comment
There was a problem hiding this comment.
A couple of comments. Mostly about keeping the files language-neutral
| private import semmle.code.cpp.ir.IR as IR | ||
| private import semmle.code.cpp.Location // TODO: SemLocation? |
There was a problem hiding this comment.
This is also the only language-specific parts of this file, right? I think we should move it out. As far as I can see, the only place where IR is used is in
this.(SemanticBound::SemSsaBound).getExpr(0) instanceof IR::PhiInstructionwhich I hope we can refactor 🤞.
| private module ConstantStage = | ||
| RangeStage<FloatDelta, ConstantBounds, CppLangImpl, RangeUtil<FloatDelta, CppLangImpl>>; | ||
|
|
||
| private module RelativeStage = | ||
| RangeStage<FloatDelta, RelativeBounds, CppLangImpl, RangeUtil<FloatDelta, CppLangImpl>>; |
There was a problem hiding this comment.
It would be good to check the performance impact of instantiating RangeStage twice. You can do that in one of two ways, I'd say:
- Run the buffer-overflows query suite we have in DCA
- Create a branch from DO NOT MERGE: C++: Replace simple range analysis uses by semantic range analysis uses #12505 and merge this PR into that branch, and run DCA on that.
There was a problem hiding this comment.
I took the liberty of starting option two here.
|
Also seeing compilation failures on CI:
|
There was a problem hiding this comment.
One small comment. Otherwise, this LGTM once DCA is happy
Edit: DCA finished and shows no new performance concerns. So my only two requests are:
- Remove the unnecessary TODO comment, and
- Get rid of the one
IR-specific construct that's now used directly inRangeAnalysisImpl.qll.
Co-authored-by: Mathias Vorreiter Pedersen <mathiasvp@github.com>
This splits the new range analysis into a constant stage and a relative stage, so that the constant stage can be used for overflow detection and potentially include some nonlinear recursion, while limiting the total performance impact by keeping relative bounds out of that recursion.